Skip to content

fix(loss): count num_valid_samples from the sample mask, not the batch dim - #3850

Open
tianyi-zhang-02 wants to merge 4 commits into
NVIDIA-NeMo:mainfrom
tianyi-zhang-02:fix-num-valid-samples
Open

tianyi-zhang-02 wants to merge 4 commits into
NVIDIA-NeMo:mainfrom
tianyi-zhang-02:fix-num-valid-samples

Conversation

@tianyi-zhang-02

@tianyi-zhang-02 tianyi-zhang-02 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Counts num_valid_samples from the effective sample_mask in the value, distillation, and cross-tokenizer distillation losses.

Workers use this metric as an inclusion gate. Counting the batch dimension lets a fully masked microbatch through even though it contributes zero loss, which dilutes step metrics. The unmasked distillation fallback still reports the batch size.

torch.count_nonzero(sample_mask) is intentional: sample_mask may contain fractional loss multipliers, but this diagnostic counts participating samples rather than summing their weights.

Validation

Current head 52035fc069da3b41a20bb0da469e4b7a324963c0, rebased onto upstream main at 90a2a212d503455d8590be5c8de3cb989d3425b0.

  • tests/unit/algorithms/test_num_valid_samples.py: 10 passed
  • covers partial, all-masked, and fractional masks through all three loss call sites
  • Ruff check, Ruff format check, and git diff --check: passed

No GPU is needed; the tests call the loss functions directly.

@tianyi-zhang-02
tianyi-zhang-02 requested review from a team as code owners August 26, 2026 18:07
@copy-pr-bot

copy-pr-bot Bot commented Aug 26, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@tianyi-zhang-02 tianyi-zhang-02 changed the title fix(loss): count num_valid_samples from the sample mask in the three losses that did not fix(loss): count num_valid_samples from the sample mask, not the batch dim Aug 26, 2026
@tianyi-zhang-02
tianyi-zhang-02 force-pushed the fix-num-valid-samples branch 2 times, most recently from 7c69abb to a3ec715 Compare August 28, 2026 15:39
@svcnvidia-nemo-ci svcnvidia-nemo-ci added the waiting-on-maintainers Waiting on maintainers to respond label Aug 28, 2026
…h dim

Six losses in this file derive num_valid_samples from the sample mask.
Three derive it from the batch dimension instead: MseValueLossFn from
values.shape[0], and both distillation losses from input_ids.shape[0].

That number is a gate, not just a log line. dtensor_policy_worker,
dtensor_policy_worker_v2 and dtensor_value_worker_v2 all do

    if num_valid_samples > 0:
        mb_losses.append(loss.item())
        all_mb_metrics.append(loss_metrics)

so a microbatch whose sample_mask is entirely zero -- which is what
overlong filtering, env-flagged masking and the critic's
seq_logprob_error_threshold filter produce -- reports the raw batch size,
passes the gate, and contributes its zero loss to the step's reported mean.
dpo.py and rm.py additionally use the value as a weighted-average
denominator.

DistillationLossFn keeps a batch-dimension fallback: its masking branch is
conditional, and with no mask every sample is valid by definition.

Tests are CPU-only -- they build the loss inputs directly rather than going
through prepare_loss_input, so no GPU and no distributed init. The
cross-tokenizer loss gets the same one-line change but no new test: every
CPU test for it calls the private helpers, and none reaches __call__ where
the metrics dict is built.

Signed-off-by: Tianyi Zhang <zhangtianyi975@gmail.com>
Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
…IA-NeMo#3496

NVIDIA-NeMo#3496 replaces DistillationLossFn's unmasked-mean fallback with a raise. Until
it lands the fallback is live and must report the batch size; after it lands
the branch is gone and the raise is the only correct behaviour.

Asserting one of them unconditionally makes the two PRs fail as a pair while
each passes alone. That is not hypothetical -- it is what happened when I
merged the whole stack onto main to check exactly this.

Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
Exercise the public cross-tokenizer loss call for partially and fully masked microbatches so the worker-facing gate is pinned to sample_mask.sum(). Keep the no-mask distillation expectation deterministic against the current main branch rather than accepting behavior from an unmerged PR.

Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-request waiting-on-maintainers Waiting on maintainers to respond

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants